Fix remaining cpp/ CodeQL code-scanning alerts in Windows build-tools#762
Conversation
maximthomas
left a comment
There was a problem hiding this comment.
Verified that all six targeted alerts (55, 57, 58, 59, 73, 75) are open on master at exactly the lines this PR touches, and that the merge ref has zero open cpp/* alerts — the goal is achieved. (The cpp/uninitialized-local alert introduced by afc3440a is already closed by 817f2b40; no action needed.) CI genuinely exercises every touched path: start-ds.bat/stop-ds.bat, windows-service.bat --enableService + net start/net stop, and an MSI install into a path with spaces.
Two comments assert a security property that Windows does not provide, and there is one silent-wrong-directory edge case.
Permission comments are factually wrong (Medium)
Neither file-creation change restricts permissions; both are pure alert suppression.
opendj-server-legacy/src/build-tools/windows/common.c:
// Create the log file readable and writable by the owner only, while
// keeping the shareable append behavior that fopen provides.Per _sopen_s docs, pmode only sets the read-only attribute at creation — it never touches the DACL: "_S_IREAD | _S_IWRITE — Reading and writing permitted. […] In the Windows operating system, all files are readable; it isn't possible to give write-only permission. Therefore, the modes _S_IWRITE and _S_IREAD | _S_IWRITE are equivalent." So this is byte-for-byte the same accessibility as the previous fopen(logFile, "a").
opendj-server-legacy/src/build-tools/windows/winlauncher.c:
// fopen_s creates the file readable and writable by the owner only,
// unlike fopen which would create it world-writable.Both halves are wrong. fopen does not create world-writable files on Windows — the DACL is inherited from the parent directory — and the fopen_s docs say nothing about restricted permissions. The one documented difference is the opposite of an improvement: "The fopen_s and _wfopen_s functions can't open a file for sharing. If you need to share the file, use _fsopen or _wfsopen […] use _SH_DENYNO for read/write sharing."
The pid file is written and closed immediately, so losing sharing is harmless in practice. Please just correct both comments and the PR description to say what is actually happening — e.g. "cpp/world-writable-file-creation models POSIX creation modes and does not apply to Windows ACLs; use the _s variants to satisfy the query". Otherwise a future maintainer will believe these files are owner-restricted when they are exactly as accessible as before. Dismissing #73/#75 as not-applicable-on-Windows is the equally valid alternative, and avoids the fopen_s sharing regression.
getCanonicalDirectoryPath silently accepts a drive-relative path (Low)
GetFullPathName docs: "If you specify U: the path returned is the current directory on the U:\ drive."
ConfigureWindowsService.getServerRoot() strips a trailing File.separator, so a drive-root install yields the string "C:". That resolves to the process's current directory on C: (for a service, C:\Windows\System32), which passes isExistingDirectory() — so the tool proceeds against an unrelated directory instead of failing. Unlikely layout, but it is the one path where the new helper silently succeeds with the wrong answer.
In opendj-server-legacy/src/build-tools/windows/common.c:
// "C:" is drive-relative: it would resolve to the current directory on
// that drive rather than to its root.
if ((strlen(path) == 2) && (path[1] == ':'))
{
debugError("The path '%s' is drive-relative.", path);
return NULL;
}Nits
- Misleading
GetLastError(): on thelength >= MAX_PATHbranch the call did not fail — the docs return the required buffer size there, andGetLastErroris only meaningful when the return value is 0. Split the two branches so the log does not report a stale error code. setInstanceDirshould bestatic: it is file-local with no prototype inservice.h, andservice.objlinks againstcommon.obj.- Inverted condition duplicates the error branch:
else if (setInstanceDir(...)) { … } else { returnCode = -1; }repeatsreturnCode = -1;five times inservice.c main.else if (!setInstanceDir(argv[2])) { returnCode = -1; } else { … }reads better and shrinks the diff. - Duplicated blocks in
winlauncher.c: thestartandstoperror handlers are byte-identical; both subcommands needargv[2]canonicalized, so hoisting the call above the branch removes ~14 lines. _instanceDirnot reset toNULLafterfree(): pre-existing, but the PR touches all five sites.isrunningdiscards its result (pre-existing): the added comment says "Check the server lock file to determine if the server is running", butrunningis never used — the subcommand returns 0 whenever the lock file is merely readable. Worth a separate fix.
…ths, deduplicate dispatch branches
|
@maximthomas Thanks for the thorough review — all points addressed in b4ee601 and 7473104. Permission comments: you are right, neither change affects the DACL. Both comments now state what actually happens — Drive-relative paths: Nits — all taken:
|
Fixes the six remaining open
cpp/*code-scanning alerts (#55, #57, #58, #59, #73, #75):cpp/path-injection(OpenDJ DSML gateway gives 404 after installation #57, Docker images after 4.3.1 are broken #58, Unable to administer Local or Remote Server with control-panel with Docker v4.3.2+ #59): the query has no string-validation barriers, so theisSafePathcheck added in Fix all cpp/ CodeQL code-scanning alerts in Windows build-tools #757 could not close these alerts. Instead the tainted flow is now cut at the source: a newgetCanonicalDirectoryPath()helper incommon.ccanonicalizes the instance directory viaGetFullPathName(resolving any./..components), rejects drive-relative paths such asC:, and requires the result to be an existing directory. It is applied to the command-line instance dir inmainof bothwinlauncher.c(start/stop) andservice.c(create/state/remove/start/isrunning, via asetInstanceDirhelper).cpp/world-writable-file-creation(Added backwards compatibility with -a #73, OpenDJ 4.4.2 and 4.4.3-SNAPSHOT build error #75): this query models POSIX creation modes, which do not exist on Windows — a file's ACL is inherited from the parent directory, and thepmodeargument of the_sfunctions only controls the read-only attribute. The actual accessibility of the pid file and debug log is therefore unchanged; the_svariants with an explicit mode simply satisfy the query. The pid file is now created withfopen_s(the sharing it denies is irrelevant for a file written and closed immediately), and the debug log with_sopen_s(..., _SH_DENYNO, _S_IREAD | _S_IWRITE)+_fdopen, keeping the shareable append behavior thatfopenprovided.cpp/poorly-documented-function(activeByDefault: Profile to build SNMP extension #55): the metric only counts comments inside the function body, so the documentation header added in Fix all cpp/ CodeQL code-scanning alerts in Windows build-tools #757 did not help;maininservice.cnow carries inline comments above the 2% threshold.Additionally, per review: the
isrunningsubcommand ofopendj_service.exenow reports the actual server state instead of discarding it — it printstrue/falseand mirrors the return-code scheme ofserviceState(0 = running, 1 = not running, 2 = cannot determine). Nothing in the repository invokes this subcommand, so the changed exit codes break no callers.